Add new output instead skip_output #637
Conversation
internal/lefthook/run.go
Outdated
| newTags := os.Getenv(envOutput) | ||
| tags := os.Getenv(envSkipOutput) | ||
|
|
||
| var logSettings log.SkipSettings | ||
| (&logSettings).ApplySettings(tags, cfg.SkipOutput) | ||
| var logSettings log.SettingsInterface | ||
|
|
||
| if tags == "" && cfg.SkipOutput == nil { |
There was a problem hiding this comment.
Let's use another name in place of 'tags'. What about outputSkipTags and outputLogTags ?
There was a problem hiding this comment.
oh, newTags was a temporary name, I just forgot about it and didn`t see it :)
Thanks, I fixed it!
internal/lefthook/run.go
Outdated
| logSettings = log.NewSkipSettings() //nolint:staticcheck //SA1019: for temporary backward compatibility | ||
| logSettings.ApplySettings(tags, cfg.SkipOutput) |
There was a problem hiding this comment.
Could you add a log.Warn telling
skip_outputis deprecated, please useoutputoption
internal/log/log.go
Outdated
| @@ -60,6 +60,19 @@ const ( | |||
| spinnerText = " waiting" | |||
| ) | |||
|
|
|||
| type SettingsInterface interface { | |||
There was a problem hiding this comment.
I think we should name it just Settings
There was a problem hiding this comment.
Let's move this interface to the settings.go?
internal/log/log.go
Outdated
| SkipSuccess() bool | ||
| SkipFailure() bool | ||
| SkipSummary() bool | ||
| SkipMeta() bool | ||
| SkipExecution() bool | ||
| SkipExecutionOutput() bool | ||
| SkipExecutionInfo() bool | ||
| SkipSkips() bool | ||
| SkipEmptySummary() bool |
There was a problem hiding this comment.
Since we move away from negation (output instead of skip_output and Settings instead of SkipSettings) I think it makes sense to change these methods to LogSuccess(), LogFailure(), .... It sounds more natural, doesn't it?
WDYT?
There was a problem hiding this comment.
yes, I agree, but I kept the same methods for compatibility with the SkipSettings and to avoid changing Skip* methods everywhere in the Lefthook code and I did not want to change other places :)
If I change methods in Settings, it means I need to change methods in SkipSettings and change it in other places, but if it is ok if I rename Skip* methods everywhere, no problem I will do it! :)
There was a problem hiding this comment.
I think it makes sense to change the naming so it is more intuitive 👍
internal/log/settings.go
Outdated
| enableAll = ^0 // Set all bits as 1 | ||
| ) | ||
|
|
||
| type Settings int16 |
There was a problem hiding this comment.
We can name it as OutputSettings if we rename the SettingsInterface to just Settings
Closes #592
⚡ Summary
Deprecate the
skip_outputand addoutputas a whitelist of output valuesI saved the
skip_outputlogic for further removal in the next versions☑️ Checklist